bootstrap collection config history store from snapshot #1460
Conversation
cendhu
force-pushed
the
bootstrap-config-store
branch
2 times, most recently
from
f761437 to
4b6adc3
Compare
| } | ||
|
|
||
| t.Run("confighistory is empty", func(t *testing.T) { | ||
| retriever := env.mgr.GetRetriever("ledger1", nil) |
There was a problem hiding this comment.
Not sure if this could cause a flake... can you change "ledger1" to "ledger0". As, the subtests are expected to be independent, better to make sure that even some reordering does not affect these.
There was a problem hiding this comment.
Also, in the similar lines, better to call cleanup() in defer as opposed to at the end. Else, one failed could cascade to others and the purpose of subtests (as opposed to a single monolithic) is defeated.
There was a problem hiding this comment.
I will make both changes. It wouldn't cause a flake because t.Run() is executed serially one after the other unless we specify t.Parallel() inside the func() present in the t.Run().
There was a problem hiding this comment.
Not sure what does t.Parallel() has to do with my comment. What I mentioned is that I am not certain if the ordering of the subtests is guaranteed? If yes, then what order is guaranteed - in the order they appear in the file or in some other order (like sort order on the name of the subtest). Even if this is guaranteed, it would be fragile such that if you swap the subtest order in the code, it would break the tests
There was a problem hiding this comment.
Agree on independence and rearrangement. My response was mainly related to the flakes issue that you raised. The t.Run() is blocked call and it runs in the same order it occurs in the main test. Only If we put t.Parallel(), the execution would be made unblocked and sub-tests can interleave. This is what I wanted to convey to mention that it wouldn't result in a flake.
| retriever := env.mgr.GetRetriever("ledger6", nil) | ||
| _, err := retriever.ExportConfigHistory(env.testSnapshotDir, testNewHashFunc) | ||
| require.NoError(t, err) | ||
|
|
There was a problem hiding this comment.
Not sure what is the significance of these lines in this test
There was a problem hiding this comment.
I don't understand. I am exporting ledger 6 so that I don't need to create some random snapshot file which would otherwise give me error reading snapshot file.... EOF
There was a problem hiding this comment.
Understood. I found it confusing because of following two reasons
- This was not needed for the immidiate following test.
- As you follow through the test code, one has to keep track of how the content in the file is getting changed.
I think that these error tests deserve to be separate subtests but I leave that to you. Otherwise, if you can produce the error causing condition explicitly (like you did at line 485) each time a fresh, that may be more consumable. In a related test, I found this pattern simpler to maintain. But as I said, it's not a blocker for me and leave to you.
There was a problem hiding this comment.
I understood your confusion now. When I add Remove() function, I will refactor this sub-test too.
| require.Contains(t, err.Error(), "error while reading from the snapshot file") | ||
| require.Contains(t, err.Error(), "confighistory.metadata: EOF") | ||
| cleanup() | ||
| }) |
There was a problem hiding this comment.
Can you add to this list one more test when the db returns an error while importing
There was a problem hiding this comment.
I added it initially to check the db writebatch. Once I added error for the iter.Next(), I couldn't test the writebatch error. I, in fact, made the maxBatchSize as var instead of constant so that the test could covered both writebatches (inside and outside the loop). I will add a test for the db error (but it would return at isEmpty() check itself)
There was a problem hiding this comment.
I understand that it's hard to skip isEmpty but whatever can be added to test, is good.
This PR builds the collection config store for a ledger using the snapshot files. FAB-18003 Signed-off-by: senthil <cendhu@gmail.com>
cendhu
force-pushed
the
bootstrap-config-store
branch
from
4b6adc3 to
0030dd0
Compare
There was a problem hiding this comment.
@cendhu - with the latest update, only one minor comment is pending but I am good to merge this and if you like to care about the pending comment, you can address later.
This PR builds the collection config store for a ledger using the snapshot files. FAB-18003 Signed-off-by: senthil <cendhu@gmail.com>
Type of change
Description
This PR adds an
ImportXXXmethod to the config history manager to allow the bootstrapping of collection config history for a given ledgerID.Additional details
Remove()method would be added in a subsequent PR and will be used during failure and recovery.Related issues
https://jira.hyperledger.org/browse/FAB-18003